-
Notifications
You must be signed in to change notification settings - Fork 73
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WaitForTopics
: let the user inject a callaback to be executed after starting the subscribers
#356
base: rolling
Are you sure you want to change the base?
Conversation
WaitForTopics
: let the user inject callables after starting subscribers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've posted some questions worth discussing here : #348 (comment)
@adityapande-1995 @clalancette I have added a more comprehensive sample code in commit 5aa51d7 |
@adityapande-1995 @clalancette |
Signed-off-by: Pintaudi Giorgio <pintaudi@axelspace.com>
Signed-off-by: Pintaudi Giorgio <pintaudi@axelspace.com>
Signed-off-by: Pintaudi Giorgio <pintaudi@axelspace.com>
3f96cb5
to
b8004ab
Compare
@osrf-jenkins retest this please |
@adityapande-1995 @clalancette How would you test a node that is subscribed to a topic with a single |
@osrf-jenkins retest this please |
@adityapande-1995 @methylDragon Until now, I have been using my fork at work for our project, but it would give us more confidence if we could just use the official repository, or even better if this PR could end up in the released version on https://index.ros.org/packages/. |
@osrf-jenkins retest this please |
@LastStarDust my apologies, taking a look now |
|
||
|
||
# TODO: Test cases fail on Windows debug builds | ||
# https://github.com/ros2/launch_ros/issues/292 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a really old issue, is it still failing :O
Signed-off-by: Giorgio Pintaudi <LastStarDust@users.noreply.github.com>
Signed-off-by: Pintaudi Giorgio <pintaudi@axelspace.com>
WaitForTopics
: let the user inject callables after starting subscribersWaitForTopics
: let the user inject callables to be executed after starting the subscribers
Signed-off-by: Pintaudi Giorgio <pintaudi@axelspace.com>
@adityapande-1995 @methylDragon I fixed some linting errors and now the checks are finally passing. |
Hello, could we get a maintainer to look at this PR? |
WaitForTopics
: let the user inject callables to be executed after starting the subscribersWaitForTopics
: let the user inject callabacks to be executed after starting the subscribers
WaitForTopics
: let the user inject callabacks to be executed after starting the subscribersWaitForTopics
: let the user inject a callaback to be executed after starting the subscribers
@methylDragon is OOO, but I will ping him to look when he is back. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested this on a backport in humble in our own CI and it is very reliable with some minor mods.
launch_testing_ros/test/examples/wait_for_topic_inject_callback.py
Outdated
Show resolved
Hide resolved
Signed-off-by: Pintaudi Giorgio <pintaudi@axelspace.com>
Signed-off-by: Pintaudi Giorgio <pintaudi@axelspace.com>
Signed-off-by: Pintaudi Giorgio <pintaudi@axelspace.com>
Signed-off-by: Pintaudi Giorgio <pintaudi@axelspace.com>
@Ryanf55 Hello, I would like to resume working on this PR. Could you please take a look at the conversations you opened and resolve the ones you think are resolved? |
@osrf-jenkins retest this please |
Yep, all of the issues we found are still problems, and we never found a good workaround for doing this generically in Sorry, we've just spent an exorbitant amount of time on this trying to get this reliable in humble and could never figure out a way that wasn't extremely coupled to knowing exactly what was running and manual graph introspection hard-coded into our test framework. I'm optimistic that upgrading distros will make it easier, but this way of testing in python is not something we're going to pursue any further unless anyone can give us a reasonable approach that works without hard-coded timeouts/sleeps for discovery that fail on slower runners. |
@Ryanf55 Thank you for sharing the situation. I am sorry we could not figure out how to make this work reliably in Humble. |
Looks like I don't have authority to resolve threads, but consider the comments OBE. I'd be happy for this to be merged as-is, and then a follow up to go in that adds the event support. |
Signed-off-by: Pintaudi Giorgio <pintaudi@axelspace.com>
…bles Signed-off-by: Pintaudi Giorgio <pintaudi@axelspace.com>
Signed-off-by: Pintaudi Giorgio <pintaudi@axelspace.com>
Signed-off-by: Pintaudi Giorgio <pintaudi@axelspace.com>
Signed-off-by: Pintaudi Giorgio <pintaudi@axelspace.com>
Signed-off-by: Pintaudi Giorgio <pintaudi@axelspace.com>
@adityapande-1995 @methylDragon @mjcarroll Here is the offending code: def trigger_callback():
command = 'ros2 topic pub --once --max-wait-time-secs 10 --keep-alive 1 \
/input std_msgs/msg/String "data: Hello World"'
p = Popen(command, shell=True, stdout=PIPE, stderr=PIPE)
stdout, stderr = p.communicate()
print("stdout: ", stdout)
print("stderr: ", stderr)
print('Callback triggered') and here is the CI output:
Please let me know how you want me to proceed. If the CI environment should be fixed or I should avoid using the |
Signed-off-by: Pintaudi Giorgio <pintaudi@axelspace.com>
That's because there is no That said, I don't think we can introduce that dependency. In particular, I'm worried about a circular dependency here, since |
Signed-off-by: Pintaudi Giorgio <pintaudi@axelspace.com>
Signed-off-by: Pintaudi Giorgio <pintaudi@axelspace.com>
21e7cf9
to
e92eb67
Compare
@clalancette |
…bles Signed-off-by: Pintaudi Giorgio <pintaudi@axelspace.com>
d6066c3
to
57277d2
Compare
@mjcarroll Friendly ping for review. |
@mjcarroll Friendly ping for review. |
I created this pull request as a proof of concept for the new feature discussed here: #348